Repository navigation
test(agents): avoid repeating every stream delta - #4171
Conversation
Castiron custom codeEvaluated main: Trusted main policy: ✅ No new custom-code files detected. 0 mixed files remain; 0 existing customizations changed. Compared A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 40372530606 --repo openai/openai-python \
--name castiron-custom-code-40372530606-1 --dir /tmp/castiron-custom-code-40372530606-1
git apply --stat /tmp/castiron-custom-code-40372530606-1/custom-code.patch
cat /tmp/castiron-custom-code-40372530606-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin a93b62c2638d41031a661d3ed9c2155860ccb9b3 eac0a17f926531004a04872b2a215dce549ce76b
python3 scripts/castiron/custom_code_report.py report \
--base a93b62c2638d41031a661d3ed9c2155860ccb9b3 \
--head eac0a17f926531004a04872b2a215dce549ce76b --target-branch main --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-eac0a17f9265
cat /tmp/castiron-custom-code-eac0a17f9265/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
marcuswood-oai
left a comment
marcuswood-oai
left a comment
There was a problem hiding this comment.
LGTM 👍 No changes requested.
The focused cache-state probe passed. The draft CI run skipped test and lint jobs; I did not independently rerun the SDK tests.
Reviewed: fc44b250a21b · Base: 43045fcaf91a · sdk-pr-review
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed fc44b250a21b3c18fd86c7490c5d2872ab60f9d5. No blocking findings. The reduced event count still exceeds the actual cache capacity. Both client modes now explicitly verify tool-event eviction and replay while retaining duplicate suppression, one tool invocation, cache bounds, and stream cleanup.
Source-only review; I did not run candidate code or tests. Hosted checks: 13 skipped, 22 success.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc44b250a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dpiet-oai
changed the title
marcuswood-oai
left a comment
marcuswood-oai
left a comment
There was a problem hiding this comment.
LGTM 👍 No changes requested.
The revised fixture preserves sustained eviction and checks duplicate suppression after the cache fills, while verifying tool-event replay without a second handler invocation in both client modes.
Reviewed: eac0a17f9265 · Base: 43045fcaf91a · sdk-pr-review
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed eac0a17. No changes requested. The fixture retains all 5,000 distinct deltas and 3,978 evictions before tool replay. Repeating the final delta still exercises duplicate suppression after sustained eviction; exact delta-list equality catches duplicate delivery. The new two-delivery assertion proves the original tool event was evicted and replayed, while single handler invocation and two inputs retain tool-call deduplication coverage in both client modes. Cache bounds and cleanup checks remain. This addresses the earlier reduced-fixture finding.
Static review of the diff, surrounding stream implementation, existing coverage and feedback, plus independent review. No candidate code or tests run locally. Current-head hosted checks: 24 successful, 15 skipped, none pending or failing (including successful OkTest).
The long agent-session stream test delivered every one of 5,000 distinct deltas twice. Keep all 5,000 distinct deltas and 3,978 cache evictions before tool replay, while repeating only the last delta to verify duplicate suppression after sustained eviction. Both client modes assert that the evicted tool event is delivered twice and its handler runs once.
Validation:
tests/lib/streaming/agents/test_streams.py(90 passed, 4 skipped); Ruff lint and format checks passed. Three serial local trials per revision on Python 3.10.16 / Pydantic 1.10.26 (DEFER_PYDANTIC_BUILD=false,pytest -n 0): median test-call times 2.64s → 1.42s sync and 2.35s → 1.20s async. Whole-gate savings have not been established.